-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: support for nested write-only arguments + write-only arguments for google_monitoring_notification_channel
#15538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
fd2c99f to
0d5ec68
Compare
|
When I do a local build, the ExactlyOneOf: []string{"sensitive_labels.0.auth_token", "sensitive_labels.0.password", "sensitive_labels.0.service_key", "sensitive_labels.0.auth_token_wo", "sensitive_labels.0.password_wo", "sensitive_labels.0.service_key_wo"},Let's discuss this feature further on this draft, I would like to validate with you if this aligns a bit with the original idea you had in mind for this feature. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
melinath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this looks like what I was imagining. It looks like the downstream generation worked fine / came out as expected, even for the case where multiple write only fields are added to the same constraint group 👍
Should be good to go once we have tests for the new fields.
Great, I will try to add these tests today! Do we also want to add some documentation about this logic somewhere? At least I can imagine without context reading this code for the first time might be a bit confusing, e.g. "why the ... do they apply this pointer logic?". On the other hand it is also not that big of a change. EDIT: I will see what I can do to make it work. I think it is a unique case in general, because this use-case uses a custom encoder and |
…to nested fields and `url_param_only: true`)
03b0d07 to
c93f109
Compare
I think I managed to address most of this in 8a48fca and ac2487d, unfortunately it is still quite some custom code, but that is also because Tests on latest commit: ➜ make testacc TEST=./google/services/monitoring TESTARGS='-run=TestAccMonitoringNotificationChannel_'
sh -c "'/Users/ramon/go/src/github.com/hashicorp/terraform-provider-google/scripts/gofmtcheck.sh'"
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC_REFRESH_AFTER_APPLY=1 TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/monitoring -v -run=TestAccMonitoringNotificationChannel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== PAUSE TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== RUN TestAccMonitoringNotificationChannel_update
=== PAUSE TestAccMonitoringNotificationChannel_update
=== RUN TestAccMonitoringNotificationChannel_updateLabels_slack
resource_monitoring_notification_channel_test.go:57:
--- SKIP: TestAccMonitoringNotificationChannel_updateLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack
resource_monitoring_notification_channel_test.go:107:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
resource_monitoring_notification_channel_test.go:216:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack (0.00s)
=== CONT TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== CONT TestAccMonitoringNotificationChannel_updateLabels
=== CONT TestAccMonitoringNotificationChannel_update
--- PASS: TestAccMonitoringNotificationChannel_notificationChannelBasicExample (6.64s)
--- PASS: TestAccMonitoringNotificationChannel_updateLabels (8.15s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabels (10.79s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo (14.25s)
--- PASS: TestAccMonitoringNotificationChannel_update (14.40s)
PASS
ok github.com/hashicorp/terraform-provider-google/google/services/monitoring 15.182sI only think the documentation for write-only arguments is still not generated fully correct. I think it makes sense to wait a little bit with that before #15385 is merged? We most likely need to add some changes to this file to enable support for the nested write-only docs: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/nested_property_documentation.html.markdown.tmpl. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
e1eb47d to
6bb8dba
Compare
2fa8999 to
bc39ae4
Compare
|
I applied some of the feedback, but not everything. I rely heavily on at least ensuring the existing acceptance tests keep succeeding with the changes. ─ ~/go/src/github.com/hashicorp/terraform-provider-google main *3 !9 ▼ 1.24.5 xxxxxx ─╮
╰─❯ make testacc TEST=./google/services/monitoring TESTARGS='-run=TestAccMonitoringNotificationChannel_' ─╯
sh -c "'/Users/ramon/go/src/github.com/hashicorp/terraform-provider-google/scripts/gofmtcheck.sh'"
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC_REFRESH_AFTER_APPLY=1 TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/monitoring -v -run=TestAccMonitoringNotificationChannel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== PAUSE TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== RUN TestAccMonitoringNotificationChannel_update
=== PAUSE TestAccMonitoringNotificationChannel_update
=== RUN TestAccMonitoringNotificationChannel_updateLabels_slack
resource_monitoring_notification_channel_test.go:57:
--- SKIP: TestAccMonitoringNotificationChannel_updateLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack
resource_monitoring_notification_channel_test.go:107:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
resource_monitoring_notification_channel_test.go:216:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack (0.00s)
=== CONT TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== CONT TestAccMonitoringNotificationChannel_updateLabels
=== CONT TestAccMonitoringNotificationChannel_update
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabels (9.99s)
--- PASS: TestAccMonitoringNotificationChannel_updateLabels (11.53s)
--- PASS: TestAccMonitoringNotificationChannel_notificationChannelBasicExample (11.75s)
--- PASS: TestAccMonitoringNotificationChannel_update (13.06s)
--- PASS: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo (18.62s)
PASS
ok github.com/hashicorp/terraform-provider-google/google/services/monitoring 19.420sLet's generate a new diff and see what is left to change, at least all acceptance tests succeed on the latest commit. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 5933 Click here to see the affected service packages
Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
@GoogleCloudPlatform/terraform-team @melinath This PR has been waiting for review for 1 week. Please take a look! Use the label |
melinath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded on the threads above.
Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
I commented on the decoder, I am slightly confused now on how to move forward, especially because it is quite a complex resource. How I understand it is that Now with the added write-only variant it even becomes more complex, because the write-only variant must also write to |
bc39ae4 to
5d25a49
Compare
|
Changed the code a little bit in 5d25a49 to make it TF_ACC_REFRESH_AFTER_APPLY=1 TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/monitoring -v -run=TestAccMonitoringNotificationChannel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== PAUSE TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== RUN TestAccMonitoringNotificationChannel_update
=== PAUSE TestAccMonitoringNotificationChannel_update
=== RUN TestAccMonitoringNotificationChannel_updateLabels_slack
resource_monitoring_notification_channel_test.go:57:
--- SKIP: TestAccMonitoringNotificationChannel_updateLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack
resource_monitoring_notification_channel_test.go:107:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabels_slack (0.00s)
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== PAUSE TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== RUN TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack
resource_monitoring_notification_channel_test.go:216:
--- SKIP: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo_slack (0.00s)
=== CONT TestAccMonitoringNotificationChannel_notificationChannelBasicExample
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabels
=== CONT TestAccMonitoringNotificationChannel_updateLabels
=== CONT TestAccMonitoringNotificationChannel_update
=== CONT TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
=== NAME TestAccMonitoringNotificationChannel_updateSensitiveLabels
resource_monitoring_notification_channel_test.go:132: Step 1/3 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# google_monitoring_notification_channel.basicauth will be updated in-place
~ resource "google_monitoring_notification_channel" "basicauth" {
id = "projects/<masked>/notificationChannels/11479684971851401096"
~ labels = {
- "password" = "************" -> null
# (2 unchanged elements hidden)
}
name = "projects/<masked>/notificationChannels/11479684971851401096"
# (7 unchanged attributes hidden)
# (1 unchanged block hidden)
}
# google_monitoring_notification_channel.pagerduty will be updated in-place
~ resource "google_monitoring_notification_channel" "pagerduty" {
id = "projects/<masked>/notificationChannels/8907246682644664376"
~ labels = {
- "service_key" = "************_key" -> null
}
name = "projects/<masked>/notificationChannels/8907246682644664376"
# (7 unchanged attributes hidden)
# (1 unchanged block hidden)
}
Plan: 0 to add, 2 to change, 0 to destroy.
=== NAME TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo
resource_monitoring_notification_channel_test.go:161: Step 1/6 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# google_monitoring_notification_channel.basicauth will be updated in-place
~ resource "google_monitoring_notification_channel" "basicauth" {
id = "projects/<masked>/notificationChannels/8739585973205083796"
~ labels = {
- "password" = "************" -> null
# (2 unchanged elements hidden)
}
name = "projects/<masked>/notificationChannels/8739585973205083796"
# (7 unchanged attributes hidden)
# (1 unchanged block hidden)
}
# google_monitoring_notification_channel.pagerduty will be updated in-place
~ resource "google_monitoring_notification_channel" "pagerduty" {
id = "projects/<masked>/notificationChannels/8907246682644665825"
~ labels = {
- "service_key" = "************_key" -> null
}
name = "projects/<masked>/notificationChannels/8907246682644665825"
# (7 unchanged attributes hidden)
# (1 unchanged block hidden)
}
Plan: 0 to add, 2 to change, 0 to destroy.
--- FAIL: TestAccMonitoringNotificationChannel_updateSensitiveLabels (14.76s)
--- FAIL: TestAccMonitoringNotificationChannel_updateSensitiveLabelsWo (15.66s)
--- PASS: TestAccMonitoringNotificationChannel_notificationChannelBasicExample (15.86s)
--- PASS: TestAccMonitoringNotificationChannel_updateLabels (17.08s)
--- PASS: TestAccMonitoringNotificationChannel_update (21.08s)
FAIL
FAIL github.com/hashicorp/terraform-provider-google/google/services/monitoring 21.898s
FAIL
make: *** [testacc] Error 1On test execution, so I kept it for now. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 5987 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🟢 All tests passed! |
This PR introduces support for nested write-only fields and adds write-only arguments for the
google_monitoring_notification_channelresource.Closing: hashicorp/terraform-provider-google#21855
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.